Skip to content

spi: add Read and separate-buffers Transfer#287

Merged
bors[bot] merged 1 commit into
rust-embedded:masterfrom
Dirbaio:spi-readwrite
Nov 4, 2021
Merged

spi: add Read and separate-buffers Transfer#287
bors[bot] merged 1 commit into
rust-embedded:masterfrom
Dirbaio:spi-readwrite

Conversation

@Dirbaio

@Dirbaio Dirbaio commented Jun 22, 2021

Copy link
Copy Markdown
Member

Depends on #286

  • Added spi::Read for only reading
  • Renamed Transfer to TransferInplace
  • Added a new Transfer, for reading+writing simultaneously with different buffers.

Open question

  • Should Transactional gain Read, Transfer (not-inplace) support? yes, done

@Dirbaio Dirbaio requested a review from a team as a code owner June 22, 2021 19:48
@rust-highfive

Copy link
Copy Markdown

r? @therealprof

(rust-highfive has picked a reviewer for you, use r? to override)

Comment thread src/blocking/spi.rs Outdated
Comment thread src/blocking/spi.rs Outdated

@eldruin eldruin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than my comments below, looks good to me, thanks!
Closes #286

Comment thread src/blocking/spi.rs Outdated
Comment thread src/blocking/spi.rs Outdated
eldruin
eldruin previously approved these changes Jun 23, 2021

@eldruin eldruin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!
Any objections @rust-embedded/hal ?

@ryankurte ryankurte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR! i'm not convinced all of these are improvements. have left a couple of comments, though it's possible i missed some discussion of this already?

Comment thread src/blocking/spi.rs Outdated
Comment thread src/blocking/spi.rs Outdated
@Dirbaio Dirbaio changed the title blocking/spi: add Read, ReadWrite, rename Transfer to ReadWriteInplace spi: add Read and separate-buffers Transfer Jun 24, 2021
@burrbull

burrbull commented Jul 7, 2021

Copy link
Copy Markdown
Member

Merge conflicts

@eldruin eldruin added this to the v1.0.0 milestone Jul 9, 2021
@lachlansneff

Copy link
Copy Markdown
Contributor

Any chance of this getting into 1.0?

@Dirbaio

Dirbaio commented Sep 21, 2021

Copy link
Copy Markdown
Member Author

Rebased. Now that Default is gone, the diff is much smaller :D

@eldruin eldruin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than my nitpicks, looks good to me, thanks!
@ryankurte do you have any further concerns about implementation-defined fill values?

Comment thread CHANGELOG.md Outdated
Comment thread src/spi/blocking.rs Outdated
Comment thread src/spi/blocking.rs Outdated

@ryankurte ryankurte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey generally seems good, one question about the updated docs and, though it appears to be minor changes, i would quite like to see a couple of impls updated to support this to make sure we're not missing anything.

Comment thread src/spi/blocking.rs Outdated
Comment thread src/spi/blocking.rs
/// The transfer runs for `max(read.len(), write.len())` words. If `read` is shorter,
/// incoming words after `read` has been filled will be discarded. If `write` is shorter,
/// the value of words sent in MOSI after all `write` has been sent is implementation-defined,
/// typically `0x00`, `0xFF`, or configurable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has this always been the case or have we implicitly been assuming read.len() == write.len()? and is this sensible to represent for more complex implementors (ie. using DMA, linux)?

we have Transactional for building sequences, if you need to do varying length things, it would seem to me to be simpler (or maybe, less surprising to demand the same lengths here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have we implicitly been assuming read.len() == write.len()?

Before this PR, only the single-buffer Transfer (now TransferInplace) existed. Single-buffer already enforced read len = write len.

The idea for the runs for max(read.len(), write.len()) comes from the nRF SPI DMA, which automatically does this. It's somewhat handy, for example to do "write command byte, read big data" you can make the write buf small (1 byte) and only need memory for a big read buf (1 + N bytes).

I agree it's not that useful if we have TransferInplace or Transactional, and it could be annoying to support using stm32 DMA for example...

If we require same-length for both bufs, do we mandate that impls panic on length mismatch, or return an error?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was discussed in today's REWG meeting. TLDR: seems people are OK with differing lengths.

Also the possibility of allowing impls to support it or not was discussed, but IMO it defeats the point since drivers wouldn't be able to use it.

Comment thread src/spi/blocking.rs
@eldruin

eldruin commented Nov 2, 2021

Copy link
Copy Markdown
Member

@Dirbaio This is good to go! Could you resolve the conflict?

@eldruin eldruin mentioned this pull request Nov 2, 2021
@Dirbaio Dirbaio force-pushed the spi-readwrite branch 2 times, most recently from 151a658 to 459e935 Compare November 3, 2021 13:25
@Dirbaio

Dirbaio commented Nov 3, 2021

Copy link
Copy Markdown
Member Author

@eldruin rebased

@eldruin

eldruin commented Nov 4, 2021

Copy link
Copy Markdown
Member

Ach, a new conflict appeared due to the default u8 PR, could you rebase again @Dirbaio ?

@Dirbaio

Dirbaio commented Nov 4, 2021

Copy link
Copy Markdown
Member Author

nooooo 🤣

@Dirbaio

Dirbaio commented Nov 4, 2021

Copy link
Copy Markdown
Member Author

@eldruin rebased

@eldruin eldruin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, thanks!
bors merge

@bors bors Bot merged commit c7497ba into rust-embedded:master Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Review is incomplete T-hal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants